Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix BitVector shifts #2731

Merged
merged 1 commit into from
May 31, 2024
Merged

Fix BitVector shifts #2731

merged 1 commit into from
May 31, 2024

Conversation

kleinreact
Copy link
Member

@kleinreact kleinreact commented May 31, 2024

Fixes #2730 such that (+>>.) and (.<<+) are compliant with (+>>) and (<<+) for bit vectors of zero length, respectively.

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

@DigitalBrains1
Copy link
Member

DigitalBrains1 commented May 31, 2024

Thanks!

We usually use Clash.Promoted.Nat.compareSNat for this. I don't know if there are any downsides to picking GHC.TypeNats.cmpNat, though. Both apparently provide proof for the comparison result, but the proof wasn't needed here in the first place since the old code type-checked.

That raises a separate point: perhaps we need to look at Bits (BitVector n) or Enum (BitVector n) if apparently one of those has some partial functions? We might need a 1 <= n on one of those. So it won't type-check anymore.

And since this is a fix that pertains to a released version of Clash, it should have a changelog entry.

[edit]
If you think: why is this guy going on about Enum (BitVector n)? Well it's because I wasn't paying attention and there is an Enum constraint on replaceBit. That constraint just didn't actually refer to the BitVector argument. I checked Enum for sanity as well; it is fine. It also wasn't germane to the issue at hand.
[/edit]

@DigitalBrains1
Copy link
Member

DigitalBrains1 commented May 31, 2024

Ah no, wait, it was in the bit shifters themselves. They call

>>> replaceBit# (0 :: BitVector 0) (-1) 1
*** Exception: replaceBit: -1 is out of range [-1..0]
CallStack (from HasCallStack):
  error, called at src/Clash/Sized/Internal/BitVector.hs:1143:12 in clash-prelude-1.8.1-72gLWAPSXgPDozrrxr9IhV:Clash.Sized.Internal.BitVector

for n = 0. It's a pity that that error message is a bit garbled. We should fix that as well, beause it's just not very nice.

[edit]
Some Bits (BitVector 0) functions raise exceptions: bit, setBit, clearBit, complementBit, testBit (all these take bit indices) and rotateL, rotateR. Those latter two give division by zero.

I suppose the ones that take bit indices are fair that they raise an exception. If you passed them a valid bit index, they'd surely work right. It's just nobody found it yet.

Those rotates I'd like to improve.
[/edit]

@kleinreact kleinreact force-pushed the fix-bitvector-shifts branch from 12437fc to fe40c2c Compare May 31, 2024 18:21
@kleinreact kleinreact marked this pull request as ready for review May 31, 2024 18:22
@kleinreact kleinreact force-pushed the fix-bitvector-shifts branch from fe40c2c to 75d8235 Compare May 31, 2024 18:54
Copy link
Member

@DigitalBrains1 DigitalBrains1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@DigitalBrains1
Copy link
Member

DigitalBrains1 commented May 31, 2024

Ah ideally it'd have FIXED: as the first word of the changelog; this helps the person writing the actual changelog for a release in quickly putting it under the right heading. And we'd like to automate a part of this one day; now it's still all manual labor.

Sorry for initially missing this. I'm rather tired, perhaps I should have just let it lie until tomorrow, in retrospect.

I don't think it's that bad if it is missing from this entry. This'll be part of 1.8.2, which will have a reasonably short changelog, I expect. It would not have been nice for 1.8.0, since that introduced a lot of changes, so that changelog was quite some work to write.

@kleinreact kleinreact force-pushed the fix-bitvector-shifts branch from 75d8235 to f96795e Compare May 31, 2024 19:27
@kleinreact kleinreact enabled auto-merge May 31, 2024 19:28
@kleinreact kleinreact merged commit 68f5f4c into master May 31, 2024
11 checks passed
@kleinreact kleinreact deleted the fix-bitvector-shifts branch May 31, 2024 20:19
@mergify mergify bot mentioned this pull request May 31, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shifting bits to 0-length BitVectors fails
2 participants